-
Notifications
You must be signed in to change notification settings - Fork 597
Fix inconsistencies on TLSRoute documentation #4139
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
/cc @mikemorris |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM aside from a minor nit about backend connections from the Gateway after TLS termination not necessarily being unencrypted.
/lgtm
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: mikemorris, rikatz The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
Thanks @mikemorris , I have fixed/tried to reword, let me know if this works |
sounds good, commited here! thank you! |
site-src/guides/tls.md
Outdated
`BackendTLSPolicy` can be used in this case to re-encrypt the connection using different | ||
set of certificate authorities, SNI and other configurations. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
BackendTLSPolicy doesn't cover TLSRoutes at this point in time. I think we'd need to discuss an addition to the enhancement.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think an update to BackendTLSPolicy may be helpful to clarify this, but the intent with this phrasing is just being explicit about where TLSRoute encryption stops:
- in Passthrough mode, TLSRoute is encrypted from the client all the way to the workload (and remains as a non-goal/out-of-scope/undefined for BackendTLSPolicy)
- in Terminate mde, TLSRoute is encrypted from the client to the gateway, and how traffic is passed from the gateway to the backend workload is an implementation detail.
BackendTLSRoute
was just mentioned as an example of how the gateway to workload traffic would not necessarily be unencrypted (some mesh-integrated gateways have alternative means of encrypting traffic from an ingress to backend workload), not imply any sort of interoperability or support, and it should be fine to rephrase or remove this mention if it's introducing unnecessary confusion.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1, maybe we can remove any mention to BackendTLSPolicy right now, and once the TLSRoute is defined we can extend BackendTLSPolicy GEP to support TLSRoute when terminating
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1, we should remove the BackendTLSPolicy
for now.
For TLSRoutes, the focus should be on the client-to-gateway connection. The gateway-to-backend connection is a separate concern and is out-of-scope for TLSRoutes, as its configuration can vary based on specific needs.
For example, even with end-to-end client-to-backend TLS (aka TLS Passthrough), a mesh deployment might still choose to re-encrypt internal traffic (see diagram below). TLSRoutes have little to none impact on gateway-to-backend connections.

Co-authored-by: Blake Covarrubias <[email protected]>
Co-authored-by: Mike Morris <[email protected]>
cdbd11a
to
278e488
Compare
site-src/guides/tls.md
Outdated
| Listener Protocol | TLS Mode | Route Type Supported | | ||
|-------------------|-------------|---------------------| | ||
| TLS | Passthrough | TLSRoute | | ||
| TLS | Terminate | TLSRoute | |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Everything else is Experimental/Core. Can we add a note that TLS Terminate for TLSRoute is extended (or implementation dependent)? Maybe as an asterisk that is explained under the table.
| TLS | Terminate | TLSRoute | | |
| TLS | Terminate | TLSRoute | * |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I missed this comment! But I have added explicitly that this is extended
Co-authored-by: Candace Holman <[email protected]>
What type of PR is this?
/kind documentation
What this PR does / why we need it:
TLSRoute documentation has some inconsistencies between what is supported, and how it is supported. This PR fixes these inconsistencies on the documentation.
Which issue(s) this PR fixes:
Fixes #1474
Does this PR introduce a user-facing change?: